-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: groupby.rank for dt64tz, period dtypes #38187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment for followon
@@ -491,8 +491,11 @@ def _ea_wrap_cython_operation( | |||
res_values, names = self._cython_operation( | |||
kind, values, how, axis, min_count, **kwargs | |||
) | |||
if how in ["rank"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, this is prob not the best location for this longer term (eg.. we should have mappings from ops -> whether they should coerce back or not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah, we have something like that in the calling class. im in the process of moving all of this wrapping to the lowest level possible (which luckily coincides with it being in the fewest places possible), xref #37494 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also be handled in maybe_cast_result_dtype
? (which basically is somewhat implementing a mapping from ops -> expected dtype)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potentially, yah. this calls simple_new instead of from_sequence, which makes it a bit of an outlier.
there's also groupby.base.cython_cast_blocklist that i think is intended for something along these lines
hmm, looks like CI is hanging, can you merge master and pin gon green. |
not merged master, but re-run an green |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
This sits on top of #38162